-
Notifications
You must be signed in to change notification settings - Fork 26.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix various Trusted Types violations without use of policy #34726
Fix various Trusted Types violations without use of policy #34726
Conversation
aab495e
to
c164fca
Compare
Stats from current PRDefault Build (Decrease detected ✓)General Overall increase
|
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
buildDuration | 15.2s | 15.1s | -87ms |
buildDurationCached | 6.1s | 6.2s | |
nodeModulesSize | 475 MB | 475 MB |
Page Load Tests Overall decrease ⚠️
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 3.497 | 3.47 | -0.03 |
/ avg req/sec | 714.87 | 720.41 | +5.54 |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.154 | 1.166 | |
/error-in-render avg req/sec | 2166.36 | 2144.45 |
Client Bundles (main, webpack)
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
925.HASH.js gzip | 179 B | 179 B | ✓ |
framework-HASH.js gzip | 42 kB | 42 kB | ✓ |
main-HASH.js gzip | 28.8 kB | 28.8 kB | ✓ |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 72.5 kB | 72.5 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.36 kB | 1.36 kB | ✓ |
_error-HASH.js gzip | 193 B | 193 B | ✓ |
amp-HASH.js gzip | 308 B | 308 B | ✓ |
css-HASH.js gzip | 327 B | 327 B | ✓ |
dynamic-HASH.js gzip | 3.08 kB | 3.08 kB | ✓ |
head-HASH.js gzip | 359 B | 359 B | ✓ |
hooks-HASH.js gzip | 920 B | 920 B | ✓ |
image-HASH.js gzip | 5.71 kB | 5.71 kB | ✓ |
index-HASH.js gzip | 263 B | 263 B | ✓ |
link-HASH.js gzip | 2.64 kB | 2.64 kB | ✓ |
routerDirect..HASH.js gzip | 320 B | 320 B | ✓ |
script-HASH.js gzip | 391 B | 391 B | ✓ |
withRouter-HASH.js gzip | 318 B | 318 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 16.3 kB | 16.3 kB | ✓ |
Client Build Manifests
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
_buildManifest.js gzip | 459 B | 459 B | ✓ |
Overall change | 459 B | 459 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
index.html gzip | 530 B | 530 B | ✓ |
link.html gzip | 544 B | 544 B | ✓ |
withRouter.html gzip | 526 B | 526 B | ✓ |
Overall change | 1.6 kB | 1.6 kB | ✓ |
Default Build with SWC (Increase detected ⚠️ )
General Overall increase ⚠️
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
buildDuration | 17.2s | 17.2s | |
buildDurationCached | 6.1s | 6.1s | -2ms |
nodeModulesSize | 475 MB | 475 MB |
Page Load Tests Overall increase ✓
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
/ failed reqs | 0 | 0 | ✓ |
/ total time (seconds) | 3.493 | 3.506 | |
/ avg req/sec | 715.76 | 713.03 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.159 | 1.151 | -0.01 |
/error-in-render avg req/sec | 2157.76 | 2172.73 | +14.97 |
Client Bundles (main, webpack)
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
925.HASH.js gzip | 178 B | 178 B | ✓ |
framework-HASH.js gzip | 42.2 kB | 42.2 kB | ✓ |
main-HASH.js gzip | 29.2 kB | 29.2 kB | ✓ |
webpack-HASH.js gzip | 1.45 kB | 1.45 kB | ✓ |
Overall change | 73.1 kB | 73.1 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
_app-HASH.js gzip | 1.35 kB | 1.35 kB | ✓ |
_error-HASH.js gzip | 179 B | 179 B | ✓ |
amp-HASH.js gzip | 312 B | 312 B | ✓ |
css-HASH.js gzip | 324 B | 324 B | ✓ |
dynamic-HASH.js gzip | 3.08 kB | 3.08 kB | ✓ |
head-HASH.js gzip | 357 B | 357 B | ✓ |
hooks-HASH.js gzip | 921 B | 921 B | ✓ |
image-HASH.js gzip | 5.76 kB | 5.76 kB | ✓ |
index-HASH.js gzip | 261 B | 261 B | ✓ |
link-HASH.js gzip | 2.76 kB | 2.76 kB | ✓ |
routerDirect..HASH.js gzip | 322 B | 322 B | ✓ |
script-HASH.js gzip | 392 B | 392 B | ✓ |
withRouter-HASH.js gzip | 317 B | 317 B | ✓ |
85e02e95b279..7e3.css gzip | 107 B | 107 B | ✓ |
Overall change | 16.5 kB | 16.5 kB | ✓ |
Client Build Manifests
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
_buildManifest.js gzip | 458 B | 458 B | ✓ |
Overall change | 458 B | 458 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | jgoping/next.js tt-violation-fixes-without-policy | Change | |
---|---|---|---|
index.html gzip | 530 B | 530 B | ✓ |
link.html gzip | 543 B | 543 B | ✓ |
withRouter.html gzip | 526 B | 526 B | ✓ |
Overall change | 1.6 kB | 1.6 kB | ✓ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, can we resolve the merge conflicts?
@ijjk thanks for taking a look. Justin has finished his internship and is probably on vacation now. Is it OK to wait for a couple of days? Otherwise I can take this over, but I will probably need to create a new PR. |
Hi @ijjk, thanks for the review! I will resolve the merge conflicts tomorrow and re-request your review once that has been done. |
Yeah definitely no hurry, thanks for the work you've done on this! |
56aa933
to
b15f852
Compare
b15f852
to
0e480f9
Compare
Linked to issue #32209.
Feature
fixes #number
contributing.md
Documentation
There are three Trusted Types violations that are fixed in this PR:
1. ban-element-innerhtml-assignments: maintain--tab-focus.ts
The innerHTML assignment here is unsafe as a string is being used that could contain an XSS attack. The solution chosen was to replace the string containing HTML with programmatically-created DOM elements. This removes the Trusted Types violation as there is no longer a string passed in that can contain an XSS attack.
Notes on solution:
<svg>
tag is omitted completely since the original snippet returns fragment.firstChild.firstChild. The first firstChild omits the<div>
, and the second firstChild omits the<svg>
, so to remove unnecessary code the created elements start at the foreignObject level.The code was tested to be equivalent by rendering both the original code and the re-written code in a browser to see if they evaluate to the same thing in the DOM. The DOM elements styles were then compared to ensure that they were identical.
2. ban-window-stringfunctiondef: packages/next/lib/recursive-delete.ts
The setTimeout function caused a Trusted Types violation because if a string is passed in as the callback, XSS can occur. The solution to this problem is to ensure that only function callbacks can be passed to setTimeout. There is only one call to the sleep function and it does not involve a string callback, so this can be enforced without breaking the application logic. In the process of doing this, promisify has been removed and the promise has been created explicitly.
The code was tested in a sample application to ensure it behaved as expected.
3. ban-window-stringfunctiondef: packages/next/client/dev/fouc.ts
This file also uses setTimeout, so the call was wrapped in a
safeSetTimeout
call that specifies that the callback argument is not a string.